-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove SolutionServices in favor of HostWorkspaceServices #62909
Remove SolutionServices in favor of HostWorkspaceServices #62909
Conversation
var services = workspace != _solutionServices.Workspace | ||
? new SolutionServices(workspace) | ||
: _solutionServices; | ||
|
||
// Note: this will potentially have problems if the workspace services are different, as some services | ||
// get locked-in by document states and project states when first constructed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is outright terrifying (and true). the workspace services are passed deeply down the solution. but when we do WithNewWorkspace, we only have a shallow pointer to the new services here. and all our children still point to the old services =-o
@@ -238,7 +240,7 @@ public async Task<MutableConflictResolution> ResolveConflictsAsync() | |||
{ | |||
var definitionLocations = _renameLocationSet.Symbol.Locations; | |||
var definitionDocuments = definitionLocations | |||
.Select(l => conflictResolution.OldSolution.GetRequiredDocument(l.SourceTree)) | |||
.Select(l => conflictResolution.OldSolution.GetRequiredDocument(l.SourceTree!)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i Have no idea why, but changing line 121 caused an NRT warning here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi So I had (not yet submitted in a PR) a change that was also touching SolutionServices and I think I saw this behavior too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you figure out what it was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -14,7 +14,7 @@ internal sealed class AdditionalDocumentState : TextDocumentState | |||
private readonly AdditionalText _additionalText; | |||
|
|||
private AdditionalDocumentState( | |||
SolutionServices solutionServices, | |||
HostWorkspaceServices solutionServices, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to rename the parameters here?
@@ -142,7 +142,7 @@ public async Task<Checksum> GetChecksumAsync(ProjectId projectId, CancellationTo | |||
}) | |||
.ToArray(); | |||
|
|||
var serializer = _solutionServices.Workspace.Services.GetRequiredService<ISerializerService>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that old code was hilarious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi Thanks for the cleanup. Was looking only since I had a PR adding something to SolutionServices so just wanted to see if there was anything interesting.
We intend to add
solutionInstance.Workspace
to the banned api list for roslyn itself. It's highly problematic for OOP, as we have to try to replicate that concept, which isn't something sensical in that world. First, OOP cannot mutation things, so exposing the mutable workspace doesn't make much sense. Second, in VS you can have many workspaces, but in OOP we map that all back to one, which means we don't have a proper representation of VS state (for example, where you might have the VSWorkspace and the Metadata-as-source workspace). This means those features either cannot use OOP, or they try to use OOP and can end up with non-sensical results.This is a series of PRs to get us off of that API so we can move to a much cleaner and simpler OOP model for the solution and services.